Retry streaming read errors in bundle download#98
Conversation
| if chunk: | ||
| fh.write(chunk) | ||
| while True: | ||
| dest_fh.write(fh.raw.read(1024*1024)) |
There was a problem hiding this comment.
I assume you want to terminate this loop at some point.
There was a problem hiding this comment.
Fixed. Still new at this programming thing.
| else: | ||
| logger.error("%s", "File {}: GET FAILED.".format(filename)) | ||
| logger.error("%s", "Response: {}".format(response.text)) | ||
| finally: |
There was a problem hiding this comment.
i think you still want the finally clause.
There was a problem hiding this comment.
This PR switches from using requests.Response.iter_content (the questionable streaming read wrapper in requests) to the Swagger client context manager, which incorporates the cleanup logic:
Lines 154 to 164 in b4ddfe2
With this logic in place, the finally clause should no longer be necessary.
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
==========================================
- Coverage 88% 87.92% -0.08%
==========================================
Files 29 29
Lines 967 969 +2
==========================================
+ Hits 851 852 +1
- Misses 116 117 +1
Continue to review full report at Codecov.
|
|
|
||
| file_path = os.path.join(dest_name, filename) | ||
|
|
||
| retries = self.get_session().adapters["https://"].max_retries |
There was a problem hiding this comment.
I'm not entirely sure I agree with this, but I don't know what's better.
There was a problem hiding this comment.
This is retrieving the retry policy that the request logic ought to apply, and applying it manually. Arguably all this machinery might be better off in the client class, but we can move it there later. Do you have an issue with the policies being the same, or...?
| from io import open | ||
|
|
||
| import requests | ||
| from requests.packages.urllib3.exceptions import ProtocolError, DecodeError, ReadTimeoutError, MaxRetryError |
3 participants
Could use some tests and also a warning in the client's streaming API autodoc, but I'm not sure how I would set up a test case, and I'd like to postpone docs until I verify that this works in the download stress tests.